[NVVM] Support - Followup enhancements #1218
Conversation
leofang
added
enhancement
|
Thanks, @abhilash1910! Any ETA to wrap this up? |
|
pre-commit.ci autofix |
|
pre-commit.ci autofix |
leofang
left a comment
leofang
left a comment
There was a problem hiding this comment.
Thanks, Abhilash! Leaving a few early feedbacks.
cuda_core/tests/test_program.py
Outdated
| bitcode_path = os.environ.get("BITCODE_NVVM_PATH") | ||
| if not bitcode_path: | ||
| pytest.skip("BITCODE_NVVM_PATH environment variable is not set.Disabling the test.") | ||
| bitcode_file = Path(bitcode_path) | ||
| if not bitcode_file.exists(): | ||
| pytest.skip(f"Bitcode file not found: {bitcode_path}") | ||
|
|
||
| if bitcode_file.suffix != ".bc": | ||
| pytest.skip(f"Expected .bc file, got: {bitcode_file.suffix}") |
There was a problem hiding this comment.
Would it be possible for us to avoid having a file locally? We have bitcode in this repo already:
cuda-python/cuda_bindings/tests/test_nvvm.py
Lines 12 to 141 in b9c76b3
so I suggest that we move it to the common place, say a new file under
cuda_python_test_helpers:https://github.com/NVIDIA/cuda-python/tree/main/cuda_python_test_helpers/cuda_python_test_helpers
and have it imported in both cuda.bindings/core tests.
There was a problem hiding this comment.
Agree on this.
There was a problem hiding this comment.
This should be partially addressed now as I have yet to remove the existing invocations from cuda_bindings test
| extra_name = f"{options.name}_extra_{i}" | ||
| nvvm.add_module_to_program(self._mnff.handle, extra_source, len(extra_source), extra_name) |
There was a problem hiding this comment.
Two notes:
- I am torn if we should allow users to specify the module names, instead of us making one up for them. Could you try my "sequence of 2-tuples" suggestion from our offline chat, and see how bad the code looks like? We should have something similar already in one of the options.
- As discussed let's check if all modules can be lazily loaded.
|
pre-commit.ci autofix |
| if self.numba_debug: | ||
| self._formatted_options.append("--numba-debug") |
There was a problem hiding this comment.
btw plz do not remove this and the related tests, however ugly this is needed for future devtool works 😅
| from contextlib import contextmanager | ||
| from dataclasses import dataclass | ||
| from typing import TYPE_CHECKING, Union | ||
| from typing import TYPE_CHECKING, List, Tuple, Union |
There was a problem hiding this comment.
I think it is safe to use normal Python list/tuple in typing now
2 participants
Description
Issue Link - #981
Changes to be addressed in this WIP PR:
{If / when it is possible to add multiple modules, a test with code that uses something from libdevice is probably a good idea.
It's also useful to be able to lazily add a module}
cc @leofang